-
-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce shinytest2 #1127
Introduce shinytest2 #1127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imprecise tests and too long test_that
. Please revisit tests and make as short test_that as possible. What is the overhead of starting headless browser here? Is it seconds or fractions of seconds?
It takes about 2 seconds to initialize the |
Let's do this inside test_that - I think it is more readable.
The point is to minimize time to detect failure and understand what is tested and what not. This file will soon have thousands of lines and imprecise test_that description (which doesn't describe expectation) will make it very hard for us developers to track the cause of the bug and monitor true-coverage. I suggest to be as precise as possible even if it affects performance (but in the same time shinytest2 can't take 10 minutes - or can?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see another round of comments. I think at least one test of the filter panel is missing. We need to test if <teal_module>$server
receives filtered data. It means you need to test teal_data
object in the <teal_module>$server
namespace and see if for example mtcars
is filtered properly.
testthat::test_that("teal_module server receives reactive teal_data object with data filtered by active filters", {
...
})
I've found it helpful to create a custom helper to define namespaces with an arbitrary number of arguments. Instead of building it manually every time. This could be integrated with WDYT? @vedhav helper_ns <- function(id) {
function(..., .css_prefix = "") {
args <- rlang::list2(...)
checkmate::assert_list(args, types = "character")
(shiny::NS(sprintf("%s%s", .css_prefix, id)))(paste(args, collapse = "-"))
}
}
ns <- helper_ns("teal-main_ui-filter_manager")
a_name <- "snapshot_manager"
output_id <- ns("filter_manager", a_name, "snapshot_add")
output_id
#> [1] "teal-main_ui-filter_manager-filter_manager-snapshot_manager-snapshot_add"
selector <- ns("filter_manager", "snapshot_manager", "snapshot_add", .css_prefix = "#")
selector
#> [1] "#teal-main_ui-filter_manager-filter_manager-snapshot_manager-snapshot_add" |
Thanks @averissimo it can be a new |
@vedhav I can take the honors 😆 |
…uce-shinytest2@main
We often just use our methods to pass parameters directly to other methods of `TealAppDriver` or `AppDriver` directly. Those already can have `{checkmate}`-like assertions on parameters' classes. However we tend to do operations on our parameters (mostly pasting) before we use them further. Now a big teal, but I would suggest checking if the input has a proper value before we paste it and pass forward. The length and the class.
Signed-off-by: Vedha Viyash <[email protected]>
# Pull Request Part of insightsengineering/coredev-tasks#503 #### Changes description - Removes `helper_NS` function - Removes outdated `skip_if_not_installed("withr")` calls --------- Co-authored-by: vedhav <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work everyone!
Looks good!
Whoa. Great job |
Part of https://github.com/insightsengineering/coredev-tasks/issues/503